livekit-api: thread and database clients - #930

🟡 Negative page sizes break list requests

For a negative list limit or page_size, _call sends that value instead of leaving the page size unset. The server receives an invalid page.pageSize, so the list request fails rather than using its default size.

Was this helpful? React with 👍 or 👎 to provide feedback.

1. [P2] Preserve numeric RPC error codes

The spec defines google.rpc.Status.code as an integer, but this implementation only preserves string codes. Falling back to HTTP status loses distinctions: ABORTED becomes already_exists; FAILED_PRECONDITION and OUT_OF_RANGE become invalid_argument; DATA_LOSS and UNKNOWN become internal. Callers cannot reliably distinguish these failures using ServerErrorCode.

Please map recognized numeric RPC codes to the existing constants before using the HTTP fallback. Keep the fallback for missing/unrecognized codes and non-JSON gateway errors, and correct the comment claiming numeric codes convey no additional information. Please add regression tests for codes sharing the same HTTP status.

2. [P2] Preserve the complete timeout configuration

Reducing ClientTimeout to a float discards connection and socket-read deadlines, replaces disabled total deadlines (None/0) with 10 seconds, and overrides a caller-supplied session's timeout when no explicit override was provided.

Please preserve the full configuration and inherit the session's timeout when there is no override. This requires adapting the transport boundary: the generated operation currently accepts a number or tuple, not ClientTimeout. Add runtime tests for session defaults, socket-read deadlines, and disabled total deadlines; assertions on the stored .total value do not cover these cases.

3. [P2] Validate nested successful-response shapes

Malformed nested payloads bypass the intended error handling. {"thread":5} leaks TypeError, malformed pageInfo leaks AttributeError, and {"thread":"bad"} or {"thread":[]} silently returns an empty Thread. The resource conversion at line 194 has the same gap.

Please validate nested resource and pagination shapes and raise ServerError(MALFORMED) with the response's HTTP status. Preserve legitimate omitted/default protobuf fields and unknown-field compatibility, and add regression coverage for these cases.

4. [API design] Settle the request-argument convention before release

Across ThreadService and DatabaseService, seven methods accept request protos while five accept keyword-only identifiers. Corresponding Get/Delete/Export request protos already exist but are unused and unexported.

Request objects would align with RoomService; direct arguments also have precedent in agent-dispatch and SIP. Please make the convention explicit and ensure the signatures, exports, and examples reflect that choice before consumers depend on this public API.

5. [Docs] Complete the public method documentation

get_thread, get_database, and list_databases have no docstrings. The remaining methods mostly provide summaries without parameter or return guidance.

Please document all twelve methods using the SDK's Args/Returns style. The documentation workflow renders these with pdoc's Google docstring format. Include relevant error behavior without implying that transport failures and input-validation errors are always ServerError.

10. [Docs, non-blocking] Explain transport and constructor differences

These services use Public API routing and do not inherit Twirp failover behavior. Their direct constructors also omit the positional failover argument accepted by the existing services.

Please document these differences and clarify which services LiveKitAPI.failover affects. Prefer explicit documentation over accepting and silently ignoring the argument. Also avoid implying that every self-hosted LiveKit server implements these endpoints merely because non-Cloud URLs are forwarded unchanged (see public_api_url's docstring).

13. [P3, non-blocking] Reject scheme-less URLs with a clear configuration error

public_api_url("x.livekit.cloud") returns the scheme-less value unchanged, leaving the failure to surface later during a request.

Could we validate supported URL schemes and report the configuration problem earlier? LiveKitAPI already rejects empty URLs; similarly clear handling would help callers diagnose malformed URLs.

14. [P3, non-blocking] Mention the new mandatory dependencies in the changeset

Lazy loading avoids importing the generated client at package startup, but pydantic, aiohttp-retry, python-dateutil, and typing-extensions still become installation requirements.

Could the changeset mention these additions so downstream users can anticipate the dependency changes?

15. [Simplification, non-blocking] Replace runtime signature inspection with explicit parameter handling

_call() inspects the generated operation's signature to discover required parameters, then infers which values to omit. This makes request behavior depend on both generated Python signatures and protobuf defaults.

Could we validate required path identifiers and normalize optional query fields explicitly at the service boundary—for example, scope=req.scope or None and a page size only when positive? The transport could then omit None query parameters, while path identifiers are validated at the call site and never omitted. This would remove reflection and make default/presence behavior easier to audit.

16. [Question, non-blocking] Document the generated/handwritten boundary

Could we document the intended boundary between generated and handwritten code? This adds 47 generated Python files, approximately 8,848 lines, for twelve operations. Requests pass through protobuf → dictionary → Pydantic → dictionary → JSON, while responses bypass generated model deserialization and are parsed into protobufs by the adapter.

The adapter also supplies session management and error handling, and service methods depend on generated operation names and request-model conversion conventions. Generation provides useful spec-derived paths, query names, and encoding; explaining that division of responsibility would help future maintainers understand which layer owns each behavior. This is a request to clarify the boundary, not to remove generated code.

nit, should it be major ? or at least minor ?

curiously, does ExportDatabase prepare the snapshot before returning, and can that exceed the 10s default for supported database sizes? Asking because the response carries a ready downloadUrl, which suggests the staging happens inline.

If so, two things would help beyond the timeout comment above: a per-call timeout override on export_database, following SipService.create_sip_participant's timeout: Optional[float] = None; and a docstring note that the snapshot scales with database size.

Separately, worth documenting that a client timeout doesn't imply the server cancelled or failed : these calls carry no X-Livekit-Request-Id, unlike the Twirp path, so a retry after a timeout isn't deduplicated. That matters most for create_database, where the docstring already notes a retry creates a second database, and for delete_database, where it'd be good to state whether deletion is complete when the call returns or eventual.

curiously, 409 in the original code can mean both already_exists and aborted, are we now binding it to already_exists and dropped aborted ?